feat(DiffView): Add CTAs and comparison presets #231
Conversation
# Conflicts: # src/pages/ProfilesExplorerView/components/SceneExploreDiffFlameGraph/components/SceneDiffFlameGraph/SceneDiffFlameGraph.tsx # src/pages/ProfilesExplorerView/components/SceneProfilesExplorer/components/Header.tsx
Unit test coverage
|
grafakus
changed the title
grafakus
changed the title
grafakus
changed the title
grafakus
changed the title
grafakus
changed the title
Does it only relate or actually fixes that issues? Is there anything else to do? |
| } | ||
| }); | ||
|
|
||
| return { |
There was a problem hiding this comment.
suggestion (non-blocking): Automatically clean up subscriptions
I think in scenes when you use this._subs.add(...) it will automatically clean up all subscriptions when a scene is deactivated. Anything added with this.subscribeToEvent(...) should also be removed automatically:
- subscribeToEvent() - clean up code
- _subs.add clean up code
There was a problem hiding this comment.
Thank you for this! I always wanted to refactor it ; I'll do it in a future PR.
| } | ||
|
|
||
| setDiffRange(diffFrom: string, diffTo: string) { | ||
| const $diffTimeRange = this.state.timeseriesPanel.state.body.state.$timeRange as SceneTimeRangeWithAnnotations; |
There was a problem hiding this comment.
thought: Should pull the state up?
Drilling down the state looks a bit cumbersome and makes testing harder do to mocking. Do you think it would make sense / be feasible to pull timeRange state up and save it only in SceneComparePanel. Timeseries panel looks for nearest timeRange anyway so will use the same one. This is not needed in this PR, just food for thought.
There was a problem hiding this comment.
As we discussed face to face: this time range is actually a timerange-like object (a hack) that we use to enable annotations on the time series (see SceneTimeRangeWithAnnotations). So it makes sense that we keep it as close as possible as where it's used (it's also responsible to change the data received from the API on-the-fly, in order to add the annotation on the time series).
Also, if in the future the platform provides a way to add range selections out-of-the-box (via a new API or so), it's likely that it's going to be configured on the timeseries panel.
Finally, for testing, I wouldn't mock anything ; I'd use end-to-end testing as we do now because these user interactions are quite complex (because they involve many different components).
...es/ProfilesExplorerView/components/SceneExploreDiffFlameGraph/SceneExploreDiffFlameGraph.tsx
Outdated
Show resolved
Hide resolved
...components/SceneExploreDiffFlameGraph/components/SceneDiffFlameGraph/SceneDiffFlameGraph.tsx
Outdated
Show resolved
Hide resolved
...w/components/SceneExploreDiffFlameGraph/components/ScenePresetsPicker/ScenePresetsPicker.tsx
Show resolved
Hide resolved
|
@ifrost I'll merge this one and work on the follow-ups tomorrow. Thank you for the review and comments! |
✨ Description
Related issue(s): resolves #224
This PR adds two call to actions when the user lands on the view:
Additionally, it introduces the new Comparison presets dropdown to help our users to quickly select different time periods:
Currently, we only provide 6 built-in presets:
📖 Summary of the changes
See diff tab.
🧪 How to test?